Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIEM][Detections Engine] Fixed minor UI bug on all rules table pagination #59094

Merged
merged 4 commits into from
Mar 3, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Mar 2, 2020

Summary

Steps to Reproduce

Go to Detections Engine --> All rules. Make sure that you have some rules added. Notice that:

  • if you only have one rule, the carrot to go to next page in pagination is not disabled
  • if you have more rules than max rows per page, pagination is enabled, but no page numbers show
  • Showing x rules always appears as 0 (thanks @MadameSheema for catching this one)

rules_list

Changes made

Found that the 'updatePagination' action in allRulesReducer was never being used, so the totalItemCount property being passed for pagination was always 0. Refactored to update pagination under setRules action and removed updatePagination.

rules_pagination_refactor

Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@yctercero yctercero marked this pull request as ready for review March 3, 2020 00:06
dispatch({
type: 'setRules',
rules: newRules,
});
dispatch({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if it will be better to add the pagination attributes in the action setRules since we need to update the pagination each time we set the rules so we can avoid two updates in our store ( meaning we will re-render twice).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that. I did that in my second commit and then switched it to this in my latest commit. I think you're right, I should put it under setRules since they'll go hand in hand. I can't think of a time where we would update rules and not pagination.

@XavierM
Copy link
Contributor

XavierM commented Mar 3, 2020

We will have to backport this in 7.6 branches to avoid any problem in the next release.

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you for fixing my mistake!!!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@yctercero yctercero merged commit 4c2aa0a into elastic:master Mar 3, 2020
yctercero added a commit to yctercero/kibana that referenced this pull request Mar 3, 2020
…ation (elastic#59094)

* Fixed minor UI bug on all rules table pagination
yctercero added a commit to yctercero/kibana that referenced this pull request Mar 3, 2020
…ation (elastic#59094)

* Fixed minor UI bug on all rules table pagination
yctercero added a commit that referenced this pull request Mar 3, 2020
…ation (#59094) (#59206)

* Fixed minor UI bug on all rules table pagination
yctercero added a commit that referenced this pull request Mar 3, 2020
…ation (#59094) (#59205)

* Fixed minor UI bug on all rules table pagination
@yctercero yctercero deleted the rules_pagination branch July 20, 2020 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants